Add Nexus APIs to the Temporal Go SDK#89
Conversation
5dd4bad to
49b1fc6
Compare
nexus/sdk-go.md
Outdated
| // RegisterNexusOperation registers an operation with a worker. Panics if an operation with the same name has | ||
| // already been registered on this worker or if the worker has already been started. A worker will only poll for | ||
| // Nexus tasks if any operations are registered on it. | ||
| RegisterNexusOperation(op nexus.RegisterableOperation) |
There was a problem hiding this comment.
I recommend registering multiple Nexus endpoints with the same worker.
There was a problem hiding this comment.
I agree we'll want this but should we do it now or start simple? I anticipate this being the common case.
There was a problem hiding this comment.
Can you clarify? Are you saying this should accept a collection instead of just requiring users to call it multiple times?
There was a problem hiding this comment.
Here's a couple of alternative's I've posted in an internal thread:
The most flexible option is to accept a nexus.Handler in WorkerOptions and have the user construct it via:
reg := &nexus.OperationRegistry{}
_ := reg.Register(op1, op2)
handler, _ = reg.NewHandler()
w := worker.New(client, "task-queue", worker.Options{NexusHandler: handler})^ This allows users to customize the handler completely and intercept calls.
The second option is to accept the registry:
reg := &nexus.OperationRegistry{}
_ := reg.Register(op1, op2)
w := worker.New(client, "task-queue", worker.Options{NexusOperationRegistry: reg})Saves a line of code but loses some flexibility and we'll have to expose interceptors in the Go SDK.
With the first option, you get multiple services OOTB, you just need a "router" handler, which we can also add to the Nexus SDK.
Here's some more alternatives for registering multiple services:
w.RegisterNexusService("service-name", reg)
// -- or --
w.RegisterNexusHandler(handler) // handle can contain a service "router"
// -- or --
w.RegisterNexusHandler("service-name", handler)Using the handler concept gives the most flexibility.
Here are some relevant definitions for reference:
https://pkg.go.dev/github.com/nexus-rpc/sdk-go@v0.0.7/nexus#Handler
https://pkg.go.dev/github.com/nexus-rpc/sdk-go@v0.0.7/nexus#OperationRegistry
https://pkg.go.dev/github.com/nexus-rpc/sdk-go@v0.0.7/nexus#Operation
https://pkg.go.dev/github.com/nexus-rpc/sdk-go@v0.0.7/nexus#RegisterableOperation
There was a problem hiding this comment.
I like
w.RegisterNexusService("service-name", reg)
and
w.RegisterNexusHandler("service-name", handler)
There was a problem hiding this comment.
Something that I don't like about forcing specifying the service name is that it creates duplication.
You already have to tell the server to map a service to a namespace and task queue using the incoming service registry APIs.
Specifying the service name also complicates the service renaming process.
So I'd suggest having a way to register a handler or operations without forcing specifying the service name but still supporting backing multiple services with a single worker for these (what I assume are) less common use cases.
If we want a single API, the accepting a Handler is the most flexible option.
There was a problem hiding this comment.
To clarify, are we discussing accepting multiple different services? Or a single service but multiple different operations?
There was a problem hiding this comment.
Multiple services on the same task queue.
There was a problem hiding this comment.
I'm currently thinking we either keep w.RegisterNexusOperation assuming multiple services on the same task queue is going to be rarely used or drop this API completely to have a single registration method.
The most flexible approach is to allow registering a handler, either with w.RegisterNexusHandler or to pass in the handler in worker options.
For a single service registration would look like this:
reg := &nexus.OperationRegistry{}
_ := reg.Register(op1, op2)
handler, _ = reg.NewHandler()
w.RegisterNexusHandler(handler)For multiple services, we'd have:
service1Reg := &nexus.OperationRegistry{}
_ = service1Reg.Register(op1, op2)
service1Handler, _ = reg.NewHandler()
service2Reg := &nexus.OperationRegistry{}
_ = service2Reg.Register(op3, op4)
service2Handler, _ = reg.NewHandler()
handler := temporalnexus.NewMuxHandler()
handler.RegisterService("svc1", service1Handler)
handler.RegisterService("svc2", service2Handler)
w.RegisterNexusHandler(handler)There was a problem hiding this comment.
I don't think we want to support registering operations without associating them to a specific service.
|
|
||
| func NewSyncOperation[I any, O any]( | ||
| name string, | ||
| handler func(context.Context, client.Client, I, nexus.StartOperationOptions) (O, error), |
There was a problem hiding this comment.
This is not very future proof if there are Temporal-specific start options we ever may want to add. Maybe change nexus.StartOperationOptions to temporalnexus.StartOperationOptions and have the latter just embed the former as its only field?
| name string, | ||
| handler func(context.Context, client.Client, I, nexus.StartOperationOptions) (O, error), |
There was a problem hiding this comment.
Consider a more future proof temporalnexus.SyncOperationOptions that accepts a name and a handler. That way you can add other options (some mutually exclusive). Do not overly concern yourself with type inference, Go does not have good type inference, don't let it change the API.
For example:
temporalnexus.NewSyncOperation(temporalnexus.SyncOperationOptions[string, string]{
Name: "get-status",
Handler: func(ctx context.Context, c client.Client, param string, opts nexus.StartOperationOptions) (string, error) {
// ...
},
})Or we could offer a shortcut which assumes ID is the param:
temporalnexus.NewSyncOperation(temporalnexus.SyncOperationOptions[string, string]{
Name: "get-status",
WorkflowQuery: "some-query",
})EDIT: Can you just add a NewSyncOperationWithOptions like you did with NewWorkflowRunOperation? Arguably we could only have the options form, but I know there is an (IMO unhealthy) obsession with type inference.
There was a problem hiding this comment.
I don't think we should over complicate the API with future compat concerns.
I don't even think that WorkflowQuery is an option we want here, I'd make a NewWorkflowQueryOperation instead if we want to go that direction but in the previous proposal, you were the one advocating for a single SyncOperation concept and giving users a client.
I'm open to renaming this to NewClientOperation or NewSyncClientOperation.
There was a problem hiding this comment.
I don't think we should over complicate the API with future compat concerns.
I don't think it's overcomplicating. Can ignore most of my comment, but I would expect NewSyncOperationWithOptions to exist just like NewWorkflowRunOperationWithOptions does. (having said that, I wish there was only 1 call that took options always, but seems you're settled on 2)
| ) | ||
|
|
||
| opGetStatus := temporalnexus.NewSyncOperation("get-status", func(ctx context.Context, c client.Client, id string, opts nexus.StartOperationOptions) (int, error) { | ||
| res, err := c.QueryWorkflow(ctx, id, "", "some-query", nil) |
There was a problem hiding this comment.
Can you confirm for me that we decided that operations cannot have arbitrary user-defined operations on themselves and that instead the id-as-parameter pattern is how you interact w/ an existing operation (besides the built-ins like cancel)? If this pattern is so common, we can have helpers for it.
There was a problem hiding this comment.
Yes, operations have a limited pre-defined interface.
| func NewWorkflowRunOperation[I, O any]( | ||
| name string, | ||
| workflow func(internal.Context, I) (O, error), | ||
| getOptions func(context.Context, I, nexus.StartOperationOptions) (client.StartWorkflowOptions, error), |
There was a problem hiding this comment.
No, you need at least a workflow ID to start a workflow.
There was a problem hiding this comment.
Can you add that this param cannot be nil in the docs? Usually people in Go don't expect to have to provide a workflow ID, so this is a change.
There was a problem hiding this comment.
Hmm.. right, they'll need to if they want their operation to be idempotent. It should be a deterministic function from input to ID.
| func MyHandlerWorkflowWithAlternativeInput(workflow.Context, MyWorkflowInput) (MyOutput, error) | ||
|
|
||
| // Alternative 1 - shortest form, for workflows that have input and outputs that map 1:1 with the operation's I/O. | ||
| opStartTransactionAlt1 := temporalnexus.NewWorkflowRunOperation( |
There was a problem hiding this comment.
To clarify, the only reason this exists is for people not wanting to make an options object? But we still make them make a full workflow start options callback? Did they really save much? Maybe instead you should have a NewWorkflowRunOperationOptions that accepts these fields instead of top-level overloads.
There was a problem hiding this comment.
Yes, that's the reason. I believe this will be the preferred way to expose workflows as operations so I made this shorthand form.
The reason I don't like the options struct is that you have to explicitly type the type params but yes a NewWorkflowRunOperationOptions also solves that. But keep in mind that even NewWorkflowRunOperationOptions may require overloads to support handler and (in the future) a result mapper.
There was a problem hiding this comment.
The reason I don't like the options struct is that you have to explicitly type the type params
I don't think this is that bad of a thing in Go and I think we're adding extra methods do to this personal dislike. But I don't have a super strong opinion just so long as we have proper options-struct-based form we can grow on all of these.
nexus/sdk-go.md
Outdated
| // RegisterNexusOperation registers an operation with a worker. Panics if an operation with the same name has | ||
| // already been registered on this worker or if the worker has already been started. A worker will only poll for | ||
| // Nexus tasks if any operations are registered on it. | ||
| RegisterNexusOperation(op nexus.RegisterableOperation) |
There was a problem hiding this comment.
Can you clarify? Are you saying this should accept a collection instead of just requiring users to call it multiple times?
| // GetOptions must be provided when setting this option. Mutually exclusive with Handler. | ||
| Workflow func(workflow.Context, I) (O, error) | ||
| // Options for starting the workflow. Must be set if Workflow is set. Mutually exclusive with Handler. | ||
| GetOptions func(context.Context, I, nexus.StartOperationOptions) (client.StartWorkflowOptions, error) |
There was a problem hiding this comment.
For others reading, the Handler term is commonly used in Go in places like net/http.Server.Handler and the GetOptions type of name is used in places like crypto/tls.Config.GetCertificate (even though it may feel inconsistent that it's not GetHandler or OptionsGetter)
cretz
left a comment
There was a problem hiding this comment.
Tentatively approve. I do want to see what the results of the worker registry thing shake out to though.
|
Short update: We're going to add a "service" concept that's a grouping of operations and rename the current "service" concept to "endpoint". This change has to start from the Nexus API spec and Go SDK and then come into this proposal. |
| } | ||
|
|
||
| // Create a [NexusClient] from a service name and an endpoint name. | ||
| func NewNexusClient(service, endpoint string) NexusClient |
There was a problem hiding this comment.
Consider reversing the order to endpoint, service.
| } | ||
|
|
||
| // Create a [NexusClient] from a service name and an endpoint name. | ||
| func NewNexusClient(service, endpoint string) NexusClient |
There was a problem hiding this comment.
Also considering adding context and an options struct to future proof this API.
cretz
left a comment
There was a problem hiding this comment.
Approving now that registration is solved
Part of the ongoing effort to integrate Nexus RPC into Temporal.
Rendered